feat(popover): add header, title, close elements #5370
Conversation
|
Preview: https://patternfly-pr-5370.surge.sh A11y report: https://patternfly-pr-5370-a11y.surge.sh |
There was a problem hiding this comment.
LGTM. Need to update
That's used here -
https://patternfly-pr-5370.surge.sh/components/card/html-demos#status-card-expanded-with-popover
And
patternfly/src/patternfly/components/LogViewer/examples/LogViewer.md
Lines 82 to 102 in 8ac660d
That's used here - https://patternfly-pr-5370.surge.sh/extensions/log-viewer#with-popover-open
| {{#if popover-title-text--attribute}} | ||
| {{{popover-title-text--attribute}}} | ||
| {{/if}}> | ||
| {{#if popover--IsAlert}} | ||
| <span class="pf-u-screen-reader"> |
There was a problem hiding this comment.
If we allow the consumer to pass their own title in, would it then be up to them to include the sr text in their title to keep it in the heading element used for the title? We're including the sr text in the heading for them now:
- https://github.com/patternfly/patternfly-react/blob/01c2451bcb6af1dc7767d86e6884c509b6b41891/packages/react-core/src/components/Popover/PopoverHeader.tsx#L39-L45
- https://github.com/patternfly/patternfly-react/blob/01c2451bcb6af1dc7767d86e6884c509b6b41891/packages/react-core/src/components/Modal/ModalBoxTitle.tsx#L56-L69
I wonder if we could include the sr text in the title wrapper instead, then we'll include it regardless the title used.
There was a problem hiding this comment.
@mcoker I don't know if that still works ok. Could this be a follow on issue so that this can move forward?
There was a problem hiding this comment.
@srambach yeah, though since it can effect the way the title updates are made in react (and other components with this structure), I wonder if we can decide now. If not, I think we can merge it and follow up. @thatblindgeye or @nicolethoen do you have an opinion on this?
We're updating the structure (in other components, too) so that the "title" element is just a container (<div>) for the title to be used. In that element, you can use the "title-text" element that PF provides to get the preferred element (heading, div, etc) and title text styling that comes from design, or you can use a <Title> (or whatever) component instead if you want to manage your own title.
Currently if the popover has an alert status, we include sr-only text in the popover title heading element. Since a user can potentially opt-out of our new title heading element (title-text) and use their own title, we can only predictably add sr-only text to the heading element if they use our title-text element. Seems like we could go a couple of ways with that.
- Document that alert status popovers (or other components with this structure) need sr-only text in the title heading element, and we include that in the title-text element. If a user is providing their own title, they'll need to be sure and include the sr-only text themselves. This is probably my preference.
- Move the sr-only text out of the heading element, and as the first thing in the title container (
<div>). I'm not sure of the implications of that since the text is no longer in the heading element.
There was a problem hiding this comment.
I think I would agree with your preference, @mcoker, that we document it and if a consumer wants to use their own custom thing, it's up to them to make sure they take any additional considerations. At least with VO, having it as part of the title-text has it announced as a single text content ("Default alert: Default popover title"), whereas moved outside of they become more separated announcements; possibly not a huge thing, but still.
One thing unrelated to this conversation is the pf-u-screen-reader class being applied. I believe we're trying to update usage of that class to just pf-screen-reader
| font-family: var(--pf-c-popover__title--FontFamily); | ||
| font-size: var(--pf-c-popover__title--FontSize); | ||
| line-height: var(--pf-c-popover__title--LineHeight); |
There was a problem hiding this comment.
Are we updating this the same way as empty state to move the title text styling to the title-text element (that can be replaced with a <Title> or whatever to opt-out of our styling)?
There is also a .pf-m-icon class modifier on __title that creates a flex layout. Since the title text will be in title-text now, I think we can just make this element a flex layout by default.
| {{/title}} | ||
| {{/popover-close}} | ||
| {{#> popover-header}} | ||
| {{#> popover-title popover-title--attribute='id="popover-bottom-header"'}} |
There was a problem hiding this comment.
@thatblindgeye wdyt about this id (used as the value for aria-labelledby on the outer popover element) - it used to be on the heading element, but this change is moving it to the "title" <div>. Is that ok? Or should it preferably be on the heading element? If the latter, I think the same isue about the sr-only text applies here, too. If we prefer the id go on the heading itself, and a user provides their own heading, it would be up to them to add the id.
There was a problem hiding this comment.
It should be okay. I think it would really make more of a difference when that screenreader text is rendered, as it could be a case of including that hidden text in the accessible name vs not. In that case it probably makes more sense having that id where it is, so that hidden text does get included in the pf-c-popover accessible name.
| <span class="pf-screen-reader"> | ||
| {{#if popover--IsDefaultAlert}} | ||
| Default | ||
| {{else if popover--IsInfoAlert}} | ||
| Info | ||
| {{else if popover--IsSuccessAlert}} | ||
| Success | ||
| {{else if popover--IsWarningAlert}} | ||
| Warning | ||
| {{else if popover--IsDangerAlert}} | ||
| Danger | ||
| {{/if}} | ||
| alert: | ||
| </span> |
There was a problem hiding this comment.
There is a partial for this class
| <span class="pf-screen-reader"> | |
| {{#if popover--IsDefaultAlert}} | |
| Default | |
| {{else if popover--IsInfoAlert}} | |
| Info | |
| {{else if popover--IsSuccessAlert}} | |
| Success | |
| {{else if popover--IsWarningAlert}} | |
| Warning | |
| {{else if popover--IsDangerAlert}} | |
| Danger | |
| {{/if}} | |
| alert: | |
| </span> | |
| {{#> screen-reader}} | |
| {{#if popover--IsDefaultAlert}} | |
| Default | |
| {{else if popover--IsInfoAlert}} | |
| Info | |
| {{else if popover--IsSuccessAlert}} | |
| Success | |
| {{else if popover--IsWarningAlert}} | |
| Warning | |
| {{else if popover--IsDangerAlert}} | |
| Danger | |
| {{/if}} | |
| alert: | |
| {{/screen-reader}} |
| | `.pf-c-popover__title-icon` | `<span>` | Creates the popover title icon | | ||
| | `.pf-c-popover__title-text` | `<span>` | Creates the popover title text | | ||
| | `.pf-c-popover__close` | `<div>` | Positions the close icon in the top-right corner of the popover. **Required if there is a close button** | | ||
| | `.pf-c-button` | `<button>` | Creates the close button for the popover. | |
| @@ -357,19 +437,20 @@ A popover is used to provide contextual information for another component on cli | |||
| | `aria-modal="true"` | `.pf-c-popover` | Tells assistive technologies that the windows underneath the current popover are not available for interaction. **Required**| | |||
| | `aria-label="Close"` | `.pf-c-button` | Provides an accessible name for the close button as it uses an icon instead of text. **Required**| | |||
| | `aria-hidden="true"` | Parent element containing the page contents when the popover is open. | Hides main contents of the page from screen readers. The element with `.pf-c-popover` must not be a descendent of the element with `aria-hidden="true"`. For more info, see [trapping focus](/accessibility/product-development-guide#trapping-focus). **Required** | | |||
| | `.pf-screen-reader` | `.pf-c-popover__title-text` | Adds text to be read saying the alert status before the title. If `.pf-c-popover__title-text` is not used to create a title, this must be added within the title. **Required for popovers that are alerts** | | |||
There was a problem hiding this comment.
It would go on a span in title-text. Here's the alert a11y docs for comparison.
| | `.pf-screen-reader` | `.pf-c-popover__title-text` | Adds text to be read saying the alert status before the title. If `.pf-c-popover__title-text` is not used to create a title, this must be added within the title. **Required for popovers that are alerts** | | |
| | `.pf-screen-reader` | `.pf-c-popover__title-text <span>` | Adds text to be read saying the alert status before the title. If `.pf-c-popover__title-text` is not used to create a title, this must be added within the title. **Required for popovers that are alerts** | |
|
🎉 This PR is included in version 5.0.0-alpha.23 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Fixes #4123
Breaking changes:
pf-c-popover__closearound the close button (required if there is a close button)pf-c-popover__headerandpf-c-popover__titleto all variations (was previously only on icon/alert variations)pf-c-popover__titleto thepf-c-popover__title-textpf-c-popover__title-textAlso removes and renames corresponding css variables.